Repository navigation
fix(host-service): name the phase in a worker task's timeout error - #6771
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughWorker task handlers can report named phases. Worker and inline execution paths retain the latest phase and include it in timeout errors. Git worktree removal reports path resolution, removal, and listing phases, with tests covering these messages. ChangesWorker task phase reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds the active phase to worker timeout errors without changing deletion behavior or timeout budgets; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GitTask as gitWorktreeRemoveTask
participant Worker as host-worker
participant Runner as WorkerTaskRunner
participant Timeout as Timeout handler
GitTask->>Worker: reportPhase("worktree-remove")
Worker->>Runner: post phase message
Runner->>Runner: store active task phase
Timeout->>Runner: task budget expires
Runner-->>Timeout: timeout error with phase
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
26caeb3 to
d4b037b
Compare
`git/removeWorktree` blows its two-minute budget in the field (HOST-SERVICE-17, HOST-SERVICE-47) and the only thing the report says is that 120000ms elapsed. The task's three steps — resolving the worktree path through a blocking realpathSync, the `worktree remove` subprocess, and the `worktree list` read after it — stall for entirely unrelated reasons, and nothing distinguished them. The budget is a timer in the parent while the steps run in the worker, so at the moment the parent gives up it knows only the elapsed time, and it retires the worker immediately afterwards. The worker therefore announces each step as it starts; the parent keeps the last announcement and appends ` in phase "<phase>"` to the timeout error, which is interpolated verbatim into the TRPCError that reaches Sentry. A task that reports no phase produces the same message as before. This changes nothing else: not the budget, not retry behaviour, not what the user sees, not the deletion logic. We still do not know why it hangs — that is what this is for. Refs HOST-SERVICE-47 Refs HOST-SERVICE-17 Claude-Session: https://claude.ai/code/session_01BrNKdKTC54LnA9jdpDDtA8
d4b037b to
1216030
Compare
|
Un-stacked from #6753 and retargeted to I originally spawned this stacked on So I cherry-picked only this PR's own commit onto main. That conflicted in one place — the hunk's context included #6753's Verified after un-stacking, since the point of the stack was a suspected dependency that turned out not to exist:
#6753 is unaffected and still merges on its own. When it does, it will conflict trivially in the same comment and resolve the same way. |
Problem
Deleting a workspace runs
git worktree removeas a worker-pool task with atwo-minute budget. On some machines the task never returns, the pool kills it,
the deletion fails, and the user retries into the same two-minute wait. The
error that reaches Sentry says only:
That is the whole report. The task does three things that stall for entirely
unrelated reasons — a blocking
realpathSync.nativeon the worktree path, thegit worktree remove --force --forcesubprocess (which recursively deletes thetree), and the
git worktree list --porcelainread afterwards — and nothingdistinguishes them. The condition has been raised as needing an owner on four
consecutive triage runs and has never been diagnosable from its own telemetry.
User-visible harm: a user deleting a workspace waits two minutes, gets a
failure, and the workspace is still there; retrying costs another two minutes.
HOST-SERVICE-17 has 192 occurrences between 2026-08-10 and today on one machine
(release 1.23.0); HOST-SERVICE-47 is the same condition on a second machine in a
different organization (6 occurrences, release 1.24.1). Both are live released
builds.
This PR does not fix that hang — we still do not know what hangs, which is
the point. It makes the next occurrence say which step was in flight.
Will this reach the reported failure? Yes, and the path is exact. The Sentry
event is the
TRPCErrorthrown atworkspace-cleanup.ts:525, and its messageinterpolates the pool's timeout text verbatim (it is also the
trpc_messageextra). Enriching that text puts the phase in the issue title and in the event
body. The captured events carry no
(inline)suffix, so they came from theworker path — which is what this instruments; the inline fallback is covered
too. Sentry groups these by stacktrace, not message (192 events with 192
different worktree paths share one group), so the existing issues keep
collecting rather than fragmenting.
Why code, versus the alternatives? Archiving is what already happened to
HOST-SERVICE-47 (
archived_until_escalating) and the condition simply came backon another machine. The code path cannot be deleted — it is the core of workspace
deletion. An inbound Sentry filter is forbidden by repo law (#6164) and would
destroy the only signal there is. Fixing at another layer — raising the budget,
retrying, making removal more tolerant — would mask the hang without explaining
it. Doing nothing is what four triage runs already did. The missing fact exists
only inside the worker at the moment it stalls, and nothing but the worker can
observe it.
No classifier or matcher is added here.
Root cause of the blind spot
The budget is enforced by a
setTimeoutin the parent (WorkerTaskRunner),while the phases run inside the worker thread. When the timer fires, the parent
knows the elapsed time and nothing else — and it immediately retires the worker,
so there is nothing left to interrogate afterwards either.
Design choice (worker reports outward, not parent reconstructs). The parent
has no way to reconstruct the phase: it holds one timer and one opaque task
payload, the handler's progress leaves no trace it can read, and the worker is
killed the instant the budget expires. The only shape that reconstructs anything
in the parent would be splitting
git/removeWorktreeinto one pool call perstep, which would change the deletion's failure semantics — a partial removal
would become an observable intermediate state the saga has to handle — and that
is out of scope for a capture-enrichment change. So the worker announces each
step as it starts, the parent keeps the last announcement, and the timeout names
it. That is one message kind, one optional callback, and one string; there is no
progress framework here, no percentages, no subscribers, and a task that reports
nothing produces byte-identical output to before.
Fix
worker-task-protocol.ts: a{kind: "phase", taskId, phase}message and itstype guard.
define-worker-task.ts: handlers get an optional second argument,reportPhase(phase). Existing handlers are untouched and ignore it.host-worker.ts: bindsreportPhaseto the running task's id.WorkerTaskRunner.ts: records the last phase per task; the timeout errorappends
in phase "<phase>"when one was reported, leaving the messageunchanged otherwise.
host-worker-pool.ts: same for the inline fallback, which enforces the samebudget and would otherwise stay undiagnosable.
tasks/git.ts:gitWorktreeRemoveTaskreportsresolve-path,worktree-remove,worktree-list. The first statement is labelled so nomoment of the handler is unlabelled — an unlabelled timeout would be
indistinguishable from one produced by a build without this change.
Nothing else changes: not the 120000ms budget, not retry behaviour, not what the
user sees, not the deletion logic.
A note on the brief: it described the middle step as "delete a leftover
directory directly". That step exists but lives in
runDestroyPhasesin theparent process, outside the worker budget, so it cannot be the step that
expires. The handler's actual three steps are the ones labelled above.
Verification
All run on this branch; output pasted verbatim.
bunx biome checkon the changed files:cd packages/host-service && bun run typecheck:bun test packages/host-service— the whole package, includingtest/workspace-cleanup.test.ts, which #6753 also edits:Proof of the outcome, not an assertion of it
Four tests in
host-worker-pool.test.tsshim agitonto the front ofPATHthat blocks on a chosen subcommand, then drive the real
gitWorktreeRemoveTaskthrough the real pool, worker thread and taskregistry — the only fakes are
gititself and the budget — and assert on theexact message the caller receives:
The two
removeWorktreecases are the same task with the same budget and thesame shim, differing only in which subcommand stalls — so a label that tracked
the task rather than the step, or one left over from the step before, fails one
of them. The third is the other half of the contract:
git/worktreeStatereports no phases, so its timeout must read exactly as it did before.
With the three
reportPhasecalls disabled, the three phase tests fail and theno-phase test still passes — they are not vacuous, and the no-phase test is not
secretly coupled to the machinery:
Two determinism notes, because a timing-dependent test that hangs a subprocess
is easy to get subtly wrong:
whole task module graph first — measured at ~305ms against a 1000ms budget on
an idle machine — which could burn the budget before the handler ran at all,
timing the task out having reported nothing. Warm, the first phase is reported
within a message hop of dispatch, so the margin stops depending on machine
load.
sleep. A fixedsleeplong enoughto survive a loaded machine is also one left running after the test, and one
short enough to clean up can expire mid-assertion and let the task succeed —
a false pass. The shim instead spins until its own file is removed, which
afterEach's existing fixture cleanup does, giving an unbounded hang withbounded cleanup.
The suite passes 3/3 under 16-way CPU contention as well as idle.
A fourth test in
test/workspace-cleanup.test.tscloses the last link — that thelabel survives into the error the reporter actually captures. It is a unit test
of the interpolation: it builds its expectation from the injected pool error
rather than restating it, so the two halves cannot drift, and the pool's exact
wording is pinned separately by the tests above.
Repo-wide
bun run lintwas not run — it hangs on cross-worktree bun locks.Not covered by a test: the
resolve-pathlabel. MakingrealpathSync.nativeblock on demand is not something a test can arrange portably, so that one line
rests on inspection.
Stacked on #6753 (
fix/delete-orphan-worktree-fallback), notmain— that PRedits
workers/tasks/git.tsandtest/workspace-cleanup.test.tstoo, and thisbuilds on top of it without reverting or restating any of it. Review the single
commit
d4b037b9a; everything below it belongs to #6753.Note for whoever merges: #6753 is a fork PR, so its branch did not exist on
superset-sh/supersetand could not be used as a base ref. The base branch hereis a mirror of #6753's head (
95c5d2112) pushed to this repo purely to make thestack expressible — delete
fix/delete-orphan-worktree-fallbackonce #6753merges, and retarget this PR at
main.Both issues stay open as their own trackers. This does not resolve the hang;
it makes the next occurrence say where it hung — hence
Refs, notFixes.Refs HOST-SERVICE-47
Refs HOST-SERVICE-17
https://claude.ai/code/session_01BrNKdKTC54LnA9jdpDDtA8
Summary by cubic
Names the in-flight phase in worker task timeout errors so triage can see which step hung. Previously the timeout named only elapsed time; now it appends the last reported phase, improving signal for
git/removeWorktreetimeouts (HOST-SERVICE-17, -47).phasemessage and guard;WorkerTaskRunnerrecords the last phase and appends it to timeout errors; the inline path does the same.define-worker-taskso handlers can accept an optionalreportPhase(phase); existing handlers need no changes.gitWorktreeRemoveTaskto reportresolve-path,worktree-remove, andworktree-list.Written for commit 1216030. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests